fix: support filter endpoint when translate backend ref. #2451
Conversation
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the ingress controller continues syncing Pods that are already terminating by skipping them, and it refines the endpoint slice lookup to handle empty slices correctly.
- Skip endpoints marked as terminating in
translateEndpointSlice - Rename
endpointtoendpointsand change nil check to empty-slice check ingetConfigsForGatewayProxy
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/provider/adc/translator/httproute.go | Add a check to skip endpoints whose Conditions.Terminating is true |
| internal/provider/adc/config.go | Rename variable and use len(endpoints) == 0 to detect empty slice |
Comments suppressed due to low confidence (1)
internal/provider/adc/translator/httproute.go:298
- Add a unit test to verify that endpoints with
Conditions.Terminating == trueare correctly skipped bytranslateEndpointSlice.
if endpoint.Conditions.Terminating != nil && *endpoint.Conditions.Terminating {
Signed-off-by: ashing <[email protected]>
| if endpoint.Conditions.Ready != nil && !*endpoint.Conditions.Ready { | ||
| log.Debugw("skip not ready endpoint", zap.Any("endpoint", endpoint)) | ||
| continue | ||
| } | ||
| if endpoint.Conditions.Terminating != nil && *endpoint.Conditions.Terminating { | ||
| log.Debugw("skip terminating endpoint", zap.Any("endpoint", endpoint)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
It is recommended to put the filter in the controller layer and not manage the status of k8s resources in the translation layer.
https://github.com/apache/apisix-ingress-controller/blob/master/internal/controller/utils.go#L1447
Signed-off-by: ashing <[email protected]>
ronething
changed the title
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
Signed-off-by: ashing <[email protected]>
| func (t *Translator) TranslateBackendRefWithFilter(tctx *provider.TranslateContext, ref gatewayv1.BackendRef, endpointFilter func(*discoveryv1.Endpoint) bool) (adctypes.UpstreamNodes, error) { | ||
| return t.translateBackendRef(tctx, ref, endpointFilter) | ||
| } | ||
|
|
||
| func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef) (adctypes.UpstreamNodes, error) { | ||
| func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef, endpointFilter func(*discoveryv1.Endpoint) bool) (adctypes.UpstreamNodes, error) { |
There was a problem hiding this comment.
These two functions are exactly the same. Can they be combined into one function?
There was a problem hiding this comment.
Currently, only the code that retrieves apisix standalone nodes calls the TranslateBackendRefWithFilter method, as it is called from another package, so it has been exported.
We can consider making a unified adjustment in the next PR.
| } | ||
|
|
||
| func (t *Translator) translateEndpointSlice(portName *string, weight int, endpointSlices []discoveryv1.EndpointSlice) adctypes.UpstreamNodes { | ||
| func (t *Translator) translateEndpointSlice(portName *string, weight int, endpointSlices []discoveryv1.EndpointSlice, endpointFilter func(*discoveryv1.Endpoint) bool) adctypes.UpstreamNodes { |
There was a problem hiding this comment.
This method seems to cover the functionality of translateEndpointSliceForIngress. It seems that translateEndpointSliceForIngress can be removed or called translateEndpointSlice in it.
There was a problem hiding this comment.
There is still a need for more clarification. It may not be appropriate to make adjustments in this PR, as it involves more code changes.
Signed-off-by: ashing <[email protected]>
4 participants
Type of change:
What this PR does / why we need it:
During the rolling update process, the ingress controller reported an error and is still attempting to synchronize the configuration for the Pods that have been taken offline.
Pre-submission checklist: